Faster HasOffsetType, HasOffsetValueType intersection - #6195
Conversation
3b94b04 to
696a0d2
Compare
|
ran the benchmarks locally and it looks really good. no side effects on the other benchmarks, and the newly added one is now wicked fast: |
|
//cc @SanderMuller please review |
…efore distributing them * `TypeCombinator::doIntersect()` now calls a new `removeDuplicateUnions()` before the `A & (B | C)` distribution, so n copies of the same union no longer get multiplied out into 2^n recursive `intersect()` calls. Restricted to the exact `UnionType` class, like the finite fast path right above it: `equals()` ignores a `TemplateUnionType`'s variance strategy, and `BenevolentUnionType` keeps its dedicated handling. * The blowup was reached through `TypeCombinator::doIntersect()`'s accessory-base-type branch: `HasOffsetType` and `HasOffsetValueType` both return `array|ArrayAccess` from `getDefaultBaseType()`, so intersecting n of them intersected n identical two-member unions. `isset()` (and `!empty()`, which narrows through `!isset()`) with 26 array offsets went from "does not finish" to 1.6 s. * Probed the sibling accessory types: every other `getDefaultBaseType()` returns a non-union (`string`, `array`, `ObjectWithoutClassType`), so `HasOffsetValueType` is `HasOffsetType`'s only twin here - it is covered by the same fix and by its own test. `array_key_exists()` chains, `isset()` on property fetches (`HasPropertyType`) and `??` chains were measured and were never affected. * Added `TypeCombinatorTest::testIntersectManyAccessoryTypesSharingAUnionBaseType` (both accessory types, asserting the unchanged result plus a wall-clock budget: 64 s before, 4 ms after), `testIntersectRepeatedUnions`, and `tests/bench/data/bug-15061.php` with the reported reproducer.
696a0d2 to
d1aee39
Compare
…s it tests/bench/data/bug-15061.php measures the same blowup end to end, so the wall-clock budget in the unit test only adds a flaky duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deduplicate where the duplicate operands are built instead of dropping repeated unions after the fact in doIntersect(): every accessory type contributes its getDefaultBaseType(), and hasOffset()/hasOffsetValue() both return `array|ArrayAccess`, so n of them handed intersect() n identical two-member unions to distribute over each other - 2^n recursive calls. Skipping a base type that an earlier accessory already contributed is sound because intersection is idempotent, and it keeps the rest of doIntersect() untouched. The reproducer from the issue goes from over three minutes (timeout) to 2.1 s, against 4.3 s for the previous placement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
staabm
left a comment
staabm
left a comment
There was a problem hiding this comment.
update PR title and description to reflect the latest state of the PR
|
Both checks are green and the tree is clean at New titleNew description
What changed versus the description currently on the PR: the fix now lives at the accessory base-type collection site instead of a |
staabm
changed the title
UnionType operands in TypeCombinator::intersect() before distributing themHasOffsetType, HasOffsetValueType intersection
2 participants
isset()with many array offsets made analysis time double with every extra offset — the reported snippet (26 optional keys of a@phpstan-typearray shape checked in oneisset()) never finished, while 13 offsets already took a minute.Root cause
Every
AccessoryTypemust be given a base type, whichTypeCombinator::doIntersect()supplies withwhere
$accessoryBaseTypesholds onegetDefaultBaseType()per accessory type. Eachisset()subject narrows the array with aHasOffsetType, andHasOffsetType::getDefaultBaseType()returns the unionarray|ArrayAccess. So n offsets built a list of n identical two-member unions, and theA & (B | C)distribution multiplies operands out one union at a time —2^nrecursiveintersect()calls before the duplicates were finally recognized at the leaves by the pairwiseisSuperTypeOf()pass.Change
src/Type/TypeCombinator.php— while collecting$accessoryBaseTypes, skip a base type that an earlier accessory already contributed (equals()). This is sound because intersection is idempotent, and it fixes the problem where the duplicate operands are built rather than cleaning them up afterwards, so the rest ofdoIntersect()is untouched.The two accessory types that return a union from
getDefaultBaseType()—HasOffsetTypeandHasOffsetValueType, botharray|ArrayAccess— are the ones that could blow up. The rest returnstring,arrayorObjectWithoutClassType; their base-type intersection was already polynomial, and deduplicating them only shortens the operand list. On the PHP level the affected constructs areisset()chains and!empty()chains (empty()narrows through!isset());array_key_exists()chains,isset($obj->x, $obj->y, …)(HasPropertyType) and($a['x'] ?? '') !== ''chains were already linear and are unchanged.The change is purely about how the answer is computed — the resulting types are identical before and after.
Measurements
Reproducer at
-l 8, 26 offsets, cold cache:@staabm's full benchmark run shows no movement on any other data file, with the new
bug-15061.phpat ~261 ms.Test
tests/bench/data/bug-15061.php— the reproducer from the issue, added to the benchmark suite the way other performance regressions in this repo are pinned. Per review feedback there is no unit-level timing test; the benchmark data file covers the same blowup end to end without a flaky wall-clock budget.make testsOK (21235 tests, 96758 assertions, 97 skipped),make phpstanno errors.Fixes phpstan/phpstan#15061